lib: refactor to use validateObject #37028

Lxxyx · 2021-01-23T04:07:02Z

No description provided.

ExE-Boss

These require(…) calls should probably all be multiline:

lib/internal/process/per_thread.js

lib/path.js

lib/internal/assert/assertion_error.js

lib/internal/streams/end-of-stream.js

lib/internal/util/inspect.js

lib/repl.js

lib/trace_events.js

lib/perf_hooks.js

lib/internal/url.js

lib/internal/streams/end-of-stream.js

Trott · 2021-01-23T19:32:17Z

Most of these represent a change in behavior where an array was previously permitted but now it is not. I don't think that's a problem, but pointing it out in case others think we should be cautious about that and maybe rate this as semver-major.

I saw a PR review comment about newlines in desructured assignments, and this would be the rule to enforce these kinds of nits. Start by just enabling the rule. We can incrementally adjust it to be more strict. Refs: https://eslint.org/docs/rules/object-curly-newline Refs: nodejs#37028 (review)

ExE-Boss · 2021-01-24T12:21:24Z

@Trott

Most of these represent a change in behavior where an array was previously permitted but now it is not.

I’m adding an allowArray option to validateObject in #37047 to prevent this from being a breaking change (and also to fix the WebIDL‑based APIs).

I saw a PR review comment about newlines in desructured assignments, and this would be the rule to enforce these kinds of nits. Start by just enabling the rule. We can incrementally adjust it to be more strict. Refs: https://eslint.org/docs/rules/object-curly-newline Refs: nodejs#37028 (review)

nodejs-github-bot · 2021-01-26T16:15:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35754/

I saw a PR review comment about newlines in desructured assignments, and this would be the rule to enforce these kinds of nits. Start by just enabling the rule. We can incrementally adjust it to be more strict. Refs: https://eslint.org/docs/rules/object-curly-newline Refs: nodejs#37028 (review) PR-URL: nodejs#37040 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2021-01-27T07:06:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35771/

nodejs-github-bot · 2021-01-27T11:11:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35774/

nodejs-github-bot · 2021-01-28T11:32:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35797/

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#37028 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 · 2021-01-28T11:56:22Z

Landed in 029d1fd

targos · 2021-01-29T11:55:14Z

lib/internal/child_process.js

-               (options === null || typeof options !== 'object')) {
-      throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
+    } else if (options !== undefined) {
+      validateObject(options, 'options');


FYI, this change broke cheerio through the workerpool module: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2604/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/cheerio_v1_0_0_rc_5/

See https://github.com/josdejong/workerpool/blob/b0e72695fc26df60e224c2d2eafc402f3097a6f4/src/WorkerHandler.js#L262

It would probably be nice to open a PR to fix that module. It seems to have been working by accident until now.

Done in josdejong/workerpool#230.

We should probably also do #37047.

I saw a PR review comment about newlines in desructured assignments, and this would be the rule to enforce these kinds of nits. Start by just enabling the rule. We can incrementally adjust it to be more strict. Refs: https://eslint.org/docs/rules/object-curly-newline Refs: #37028 (review) PR-URL: #37040 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #37028 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

kanongil · 2021-03-03T12:23:45Z

The workerpool breakage is more serious than fixing the head version. My project uses both v2 and v3 through various dependencies I have little control over.

Trott · 2021-03-03T14:45:52Z

The workerpool breakage is more serious than fixing the head version. My project uses both v2 and v3 through various dependencies I have little control over.

Ooof. The fix is in workerpool@6.1.0, released about one month ago.

Unfortunately, workerpool@6.1.0 accounts for less than 1/3 of the package's over 2.2 million downloads in the last week.

I saw a PR review comment about newlines in desructured assignments, and this would be the rule to enforce these kinds of nits. Start by just enabling the rule. We can incrementally adjust it to be more strict. Refs: https://eslint.org/docs/rules/object-curly-newline Refs: #37028 (review) PR-URL: #37040 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 23, 2021

ExE-Boss reviewed Jan 23, 2021

View reviewed changes

aduh95 approved these changes Jan 23, 2021

View reviewed changes

lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved

RaisinTen approved these changes Jan 23, 2021

View reviewed changes

Trott approved these changes Jan 23, 2021

View reviewed changes

Trott mentioned this pull request Jan 23, 2021

tools: enable object-curly-newline in ESLint rules #37040

Merged

Lxxyx force-pushed the refactor-to-use-validate-object branch from 5964a61 to d777cbc Compare January 24, 2021 06:54

ExE-Boss mentioned this pull request Jan 24, 2021

lib: fix WebIDL object and dictionary type conversion #37047

Merged

Lxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2021

jasnell approved these changes Jan 27, 2021

View reviewed changes

Lxxyx force-pushed the refactor-to-use-validate-object branch from 14eca5f to 3686855 Compare January 28, 2021 05:16

This comment has been minimized.

Sign in to view

aduh95 force-pushed the refactor-to-use-validate-object branch from 3686855 to 029d1fd Compare January 28, 2021 11:55

aduh95 closed this Jan 28, 2021

aduh95 merged commit 029d1fd into nodejs:master Jan 28, 2021

Lxxyx deleted the refactor-to-use-validate-object branch January 28, 2021 13:47

targos reviewed Jan 29, 2021

View reviewed changes

targos mentioned this pull request Feb 2, 2021

v15.8.0 release proposal #37183

Merged

targos added the backport-blocked-v14.x label May 25, 2021

loqs mentioned this pull request Mar 8, 2022

Unable to build Nomad with Nodejs v16 LTS hashicorp/nomad#11795

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: refactor to use validateObject #37028

lib: refactor to use validateObject #37028

Lxxyx commented Jan 23, 2021

ExE-Boss left a comment •

edited

Loading

Trott commented Jan 23, 2021

ExE-Boss commented Jan 24, 2021

nodejs-github-bot commented Jan 26, 2021

nodejs-github-bot commented Jan 27, 2021

nodejs-github-bot commented Jan 27, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 28, 2021

aduh95 commented Jan 28, 2021

targos Jan 29, 2021

aduh95 Jan 29, 2021

ExE-Boss Jan 29, 2021

kanongil commented Mar 3, 2021

Trott commented Mar 3, 2021

lib: refactor to use validateObject #37028

lib: refactor to use validateObject #37028

Conversation

Lxxyx commented Jan 23, 2021

ExE-Boss left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Jan 23, 2021

ExE-Boss commented Jan 24, 2021

nodejs-github-bot commented Jan 26, 2021

nodejs-github-bot commented Jan 27, 2021

nodejs-github-bot commented Jan 27, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 28, 2021

aduh95 commented Jan 28, 2021

targos Jan 29, 2021

Choose a reason for hiding this comment

aduh95 Jan 29, 2021

Choose a reason for hiding this comment

ExE-Boss Jan 29, 2021

Choose a reason for hiding this comment

kanongil commented Mar 3, 2021

Trott commented Mar 3, 2021

ExE-Boss left a comment •

edited

Loading